fix(relinker): discover required guest module identities - #1503

Verified on Linux against today's main (4b005b22, i.e. well past the 084aeeb6 this was rebased onto). Everything below is a re-run of your own commands, not a read of the diff.

Your CI claim holds. Run 37767916497 is head_sha 7d3b3930…, event=pull_request, branch fix/guest-required-identities, linux and windows both success — a real pull_request run for this exact head, not a main push.

Merge and tests

  merge onto 4b005b22            no conflicts, 6 files, +359/-7
  registered test                guest_module_identity (ctest #457) — a reconfigure is needed
                                 to pick it up, since add_test lands in CTestTestfile.cmake
  Linux run                      Passed 0.08 s
  NEGATIVE CONTROL               implementation reverted to main (the 4 source files),
                                 test kept  ->  FAILED, AssertionError: assert artifact.is_file()
                                 restored                                  ->  Passed
  all relinker groups, Linux     37/37 passed in 5.4 s   (39 names in CMakeLists; the 2 that do
                                 not run here are amd64_only_windows and windows_dependency_diagnostics)

The negative control is the part I care about most, and it passes honestly: with GuestImageReader.cpp, GuestModuleBuilder.cpp and the two headers back at main the regression fails on the missing provider artifact, so the new test does gate this change rather than merely passing beside it.

The stacked pair lands cleanly — I checked, because #1617 is by you and touches the same four files

  merge #1503 onto 4b005b22      no conflicts
  merge #1617 on top             no conflicts
  both new tests                 guest_module_identity Passed, import_library_identity Passed
  relinker groups with both      38/38 passed in 5.4 s

So this is not a duplicate-PR situation: git merge-base --is-ancestor pr-1503 pr-1617 is true, the shared lines are inherited rather than duplicated, and the order in #1617's body ("merge #1503 first") replays without conflict on today's main. The maintainer can land the pair as-is.

One substantive point: the two dependency walks disagree.

  :53    if (Io::ReadU64(...) != 1) continue;                              // tolerates
  :272   if (Io::ReadU64(...) != 1) throw ...("Unexpected executable dependency tag");  // throws

SysVDynamicSectionBuilder.cpp:155 appends only DT_NEEDED (for (const std::uint32_t off : neededOffsets) _appendDynEntry(result.DynamicSegmentData, DT_NEEDED, off);), so the table cannot contain another tag today and the continue is unreachable. That is exactly why I would keep throw in both: the deleted ReadNeededNames was the single place that held that invariant, and it is now inlined twice with different guards, one of which silently drops a dependency instead of failing. main threw here, and the repo's rule is that unimplemented or impossible paths throw rather than pass quietly. Restoring the helper (taking the name list as the walked output) would remove the asymmetry and the duplication at once.

Two smaller things, both labelled as such:

1. Latent, not reachable today. paths is never deduplicated, and the image loop now does auto image = std::move(discovered.at(path));. A repeated path would move out of an already-moved-from GuestImage (empty Bytes, empty OutputName) where main re-read the file, so the failure mode changes from "same image twice" to "empty image". Right now the module-directory scan, the found map and the identity map exclude one another, so I could not construct a duplicate — a std::unique after the std::sort at :120 would make that independent of the scan logic.
2. Question. The identity scan accepts .sprx (extension != ".prx" && extension != ".sprx" && extension != ".suprx"), but matchesIdentity only tests name == identity, identity + ".prx" and identity + ".suprx". A needed foo.sprx therefore can never match a declared identity foo. Deliberate, or should .sprx be in the matcher as well?

Two process notes, neither about the code. The only review on this PR is an APPROVED from @klosixgaming-arch, the same account that approved #1511; no maintainer has looked at it yet, and its head is one of the 38 in the CI-approval backlog (#1631) — for this head the run did execute, so this is purely a matter of a review pass. Nothing here needs your action beyond the continue/throw decision.

Written with AI assistance.

@Dextroid17 Thanks for running the regressions independently. Addressed in 188dd9f:

• Merged main 4b005b2, which already contains refactor(relinker): read the executable's DT_NEEDED entries in one place #1403's shared, strict ReadNeededNames helper. Both dependency walks now use it.
• Deduplicated the sorted paths before moving parsed images.
• Added .sprx identity matching and import normalization, with executable/guest and standard/root/nested provider cases. The extended regression fails on 7d3b393 at missing libGuest.sprx and passes on the update.

All 37 registered relinker groups pass locally on Windows, along with conventions and diff checks. The updated dependent #1617 passes 38 groups and retains its failing parent negative control. The descriptions now distinguish these results from historical title runs; CI for the new heads is still running.

AI-assisted: yes

Re-checked 188dd9f5 independently — all three points landed, and I confirmed each in the tree rather than from the message:

  continue/throw   ReadNeededNames is back as the single helper (GuestModuleBuilder.cpp:17);
                   the one remaining walk calls it at :69 and throws at :21 on a
                   non-DT_NEEDED tag. The two differently-guarded copies are gone, so the
                   asymmetry I flagged no longer exists anywhere in the tree.
  dedup            :132 std::sort + :133 paths.erase(std::unique(...)) — before the move at :90.
  .sprx            matcher :92 now tests name == identity || +".prx" || +".sprx" || +".suprx",
                   and ImportModule.hpp:13/:41 normalize ".sprx"/".suprx" to the ".prx" spelling.

The .sprx question is answered the way I hoped: the matcher was the half that could never match a foo.sprx request, and it is now symmetric with the scan on both the identity and the normalization side, with the cases pinned in test_guest_module_identity.py.

Nothing further from me on the code — the three review points are discharged.

One thing that is not the code, and it is yours to decide. The Relinker (macos-latest) leg on this head reads fail, but the job is conclusion: cancelled after 20m31s with zero steps recorded (started 16:53:58 → completed 17:14:29) — a runner that never produced a step, not a test that ran and failed. The same leg passed in 1m05s for the sibling #1617, so the code path is not implicated. If it re-runs green after a re-run, ignore this; if it keeps cancelling, it is worth a note to the owner since it is a red check on your PR.

Written with AI assistance.

The macOS Relinker job on 188dd9f5 was cancelled without recording any steps; the Linux and Windows jobs passed. I cannot re-run it from this contributor account: neither the run nor job menu offers a re-run action, and the Actions API returns 403. Could someone with Actions permission re-run the cancelled macOS job?

AI-assisted: yes

Confirmed the permission wall independently — I cannot re-run it either. My account's repo permissions are pull: true, push: false, maintain: false, admin: false, so the re-run API is 403 for me as well; a re-run needs someone with write access. So this one does sit with @boykopovar or any other maintainer.

I did check the run rather than just the badge, and it confirms your read:

run 37812367472 (Build, 188dd9f5)   job                     result      steps   window
                                    linux                   success      12     16:59:12 -> 17:08:26
                                    windows                 success      15     16:54:17 -> 17:01:06
                                    Relinker (ubuntu-latest) success     10     16:59:25 -> 17:00:19
                                    Relinker (windows-latest) success    11     17:01:08 -> 17:03:27
                                    Relinker (macos-latest)  cancelled     0     16:53:58 -> 17:14:29

Every job in that run is success except the macOS leg, and that leg recorded no step at all over 20m31s — the job carries timeout-minutes: 15, so it never reached a step, which reads as a runner that was never allocated rather than anything the code did. The failure on the run is that one job and nothing else.

It is also not a repo-wide macOS problem right now: a Build run on main at 17:45 (37819086958) has its Relinker (macos-latest) leg success with 10 steps, so macOS runners are allocating normally and this looks like a one-off allocation miss on this head. Nothing in the diff is implicated by it.

So: the code and the other four jobs are clean, and the only missing piece is a maintainer pressing re-run on that leg.

Written with AI assistance.

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

For the cancelled macOS job: a contributor account cannot re-run it, but a fork can run the same workflow itself — the workaround was documented in #1631:

gh workflow run build.yml -R <your-fork>/AnyPS5 --ref <branch>

• Actions must be enabled on the fork (Settings → Actions → Allow all actions); macOS runners are free for public repositories.
• The run shows in your fork's Actions tab, not in this PR's checks — paste the run URL here when it finishes and it still counts as your evidence.
• Disclosure worth stating in the same comment: it builds your branch head, not the merge commit, and a fork has no ccache, so it is slower than this repository's job.

The missing macOS result is now available for unchanged head 188dd9f5f814d2f258c5ac04775ba42ec80374c5: fork Build run, macOS job. The Release relinker build succeeds and all 35 registered tests pass in 9.20 s, including guest_module_identity in 0.55 s.

This runs the branch head through the existing workflow, not a merge with current main. It is separate evidence and leaves the cancelled upstream check unchanged. The fork run is now complete: both full-runtime build/test jobs (Linux and Windows) and all three relinker jobs (Linux, Windows and macOS) pass. No code or workflow changes were needed. The PR body now links the result.

AI-assisted: yes

188dd9f merges main into the branch; please rebase onto main and force-push instead. PR branches here don't carry merge commits.

Rebased and squashed onto main 3bd74c15 as c7a2eea7, with no merge commits in the PR range. The resulting tree preserves the reviewed 188dd9f5 correction. Dependent #1815 is rebased onto this parent as 0c3d0434.

Final-commit Windows relinker builds and conventions pass. The parent passes 39/40 registered groups and the child 40/41 within the 30-second limit; the unchanged TLS group times out on both and passes separately in 42.70 s and 33.13 s. These limits are disclosed in both descriptions. The optional AMD assembly case is skipped locally.

CI is now complete for both heads: Linux/macOS relinker jobs pass; Windows passes our regressions and TLS but fails the unchanged upstream hardware_oracle test because its clang lacks the amdgcn-amd-amdhsa target. Parent Windows log, child Windows log. Full-runtime jobs were skipped. No unrelated oracle change or CI retry was added to these PRs.

Prepared with AI assistance.

Rebase and CI status noted — and thank you for disclosing the 30-second limit and the Windows hardware_oracle failure as unchanged upstream rather than hiding it or retrying it away. A run that fails on a test the change does not touch is exactly the kind of thing that should be stated plainly.

I have not re-run the relinker suites on c7a2eea7 myself; my earlier pass was against 188dd9f5, which you say the squashed tree preserves, so I am taking that on your word rather than confirming it. If an independent run would help, say so and I will build the relinker and the guest-module groups on the new head — relinker work does not depend on the GPU, so I can do it properly here.

Written with AI assistance.

Upstream #2151 has now fixed the unsupported AMDGPU-target probe. Rebased onto main d70b8998 as 98c47df9; dependent #1815 is now 7efd5888. Both contribution patches are unchanged (git range-diff), with no merge commits in their ranges.

The new #1503 CI and #1815 CI pass all three relinker platforms and conventions. Windows passes 41/41 and 42/42 registered groups, with TLS at 7.39 s and 9.27 s. These runs test the PR merges into that main; full-runtime jobs were skipped.

The local 30-second TLS limit still fails on both; separate diagnostic runs finish in 33.41 s and 31.51 s. The optional AMD assembly case skips locally. Both descriptions retain these limits and distinguish the earlier title evidence from this rebase.

Prepared with AI assistance.

Checked on Linux, merged onto current main: relinker build and ctest 43/43 pass; guest_module_identity fails with the source changes reverted and passes with them. Relinker jobs (Linux/Windows/macOS) and conventions are green on this head. Merging; #1782 and #1830 touch the same lines in GuestModuleBuilder.cpp, so whichever lands next needs a rebase.